fix: ensure price update follows oracle module after UpdateTokenMetadata in fee abstraction#322
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughProtobufs updated: Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
x/feeabstraction/types/params.go (1)
138-149: Constructor hardcodesEnabled: true, limiting flexibility.
NewUpdateTokenMetadataalways setsEnabled: true, but callers might need to create metadata withEnabled: false(e.g., to disable a fee token via governance). Consider adding anenabledparameter or using struct literal initialization where control is needed.♻️ Proposed fix to add enabled parameter
// NewUpdateTokenMetadata creates a new fee token with the given denom and decimals func NewUpdateTokenMetadata( denom, oracleDenom string, decimals uint32, + enabled bool, ) UpdateTokenMetadata { return UpdateTokenMetadata{ Denom: denom, OracleDenom: oracleDenom, Decimals: decimals, - Enabled: true, + Enabled: enabled, } }Note: This would require updating all call sites to pass the
enabledparameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/feeabstraction/types/params.go` around lines 138 - 149, The constructor NewUpdateTokenMetadata currently hardcodes Enabled: true; change its signature to accept an enabled bool (e.g., NewUpdateTokenMetadata(denom, oracleDenom string, decimals uint32, enabled bool)) and set UpdateTokenMetadata.Enabled to that parameter so callers can create disabled tokens, then update all call sites to pass the appropriate enabled value (or provide an additional convenience helper if you want a default-true factory). Ensure you update references to NewUpdateTokenMetadata and the UpdateTokenMetadata struct initialization accordingly.x/feeabstraction/keeper/msg_server_test.go (1)
224-230: Consider adding an assertion for theEnabledfield.The test verifies
Denom,OracleDenom,Decimals, andPrice, but doesn't check thatEnabledis correctly persisted. This would help catch the current bug where theEnabledfield from the input message is dropped.♻️ Proposed enhancement
for i, token := range tokens.Items { s.Require().Equal(tc.msg.Tokens.Items[i].Denom, token.Denom) s.Require().Equal(tc.msg.Tokens.Items[i].OracleDenom, token.OracleDenom) s.Require().Equal(tc.msg.Tokens.Items[i].Decimals, token.Decimals) s.Require().True(token.Price.IsZero(), "expected price to be 0 for denom %s, got %s", token.Denom, token.Price) + s.Require().Equal(tc.msg.Tokens.Items[i].Enabled, token.Enabled, "expected Enabled to match for denom %s", token.Denom) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/feeabstraction/keeper/msg_server_test.go` around lines 224 - 230, The test loop in msg_server_test.go that iterates over tokens.Items checks Denom, OracleDenom, Decimals and Price but omits asserting the Enabled field; add an assertion inside the same for i, token := range tokens.Items loop comparing tc.msg.Tokens.Items[i].Enabled with token.Enabled (e.g., s.Require().Equal(tc.msg.Tokens.Items[i].Enabled, token.Enabled)) so the test fails if Enabled is not persisted by the handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x/feeabstraction/keeper/msg_server.go`:
- Around line 113-119: The code in msg_server.go zeroes token prices by calling
types.NewFeeTokenMetadata which hardcodes Enabled=true, dropping the input
message's Enabled field; change the construction of resetItems inside the loop
that processes msg.Tokens.Items so it preserves token.Enabled while only setting
Price to math.LegacyZeroDec() (e.g., create FeeTokenMetadata using a struct
literal or a constructor that accepts an Enabled argument instead of
NewFeeTokenMetadata), ensuring the Denom, OracleDenom, Decimals are copied from
token and Enabled uses token.Enabled before building resetFeeTokens.
In `@x/feeabstraction/types/msg_test.go`:
- Around line 82-88: The test "valid - fee tokens" currently constructs an empty
collection with types.NewUpdateTokenMetadataCollection(), duplicating the "valid
- empty fee tokens" case; replace that empty collection with a non-empty
collection containing one or more valid token metadata entries so the test
actually exercises the non-empty path: build a collection using the project API
(e.g., create one or more types.UpdateTokenMetadata objects and add them to
types.NewUpdateTokenMetadataCollection() or construct the collection with those
entries) and pass it into
types.NewMessageUpdateFeeTokens(authtypes.NewModuleAddress(govtypes.ModuleName).String(),
<non-empty collection>) so the test differs from the empty case.
---
Nitpick comments:
In `@x/feeabstraction/keeper/msg_server_test.go`:
- Around line 224-230: The test loop in msg_server_test.go that iterates over
tokens.Items checks Denom, OracleDenom, Decimals and Price but omits asserting
the Enabled field; add an assertion inside the same for i, token := range
tokens.Items loop comparing tc.msg.Tokens.Items[i].Enabled with token.Enabled
(e.g., s.Require().Equal(tc.msg.Tokens.Items[i].Enabled, token.Enabled)) so the
test fails if Enabled is not persisted by the handler.
In `@x/feeabstraction/types/params.go`:
- Around line 138-149: The constructor NewUpdateTokenMetadata currently
hardcodes Enabled: true; change its signature to accept an enabled bool (e.g.,
NewUpdateTokenMetadata(denom, oracleDenom string, decimals uint32, enabled
bool)) and set UpdateTokenMetadata.Enabled to that parameter so callers can
create disabled tokens, then update all call sites to pass the appropriate
enabled value (or provide an additional convenience helper if you want a
default-true factory). Ensure you update references to NewUpdateTokenMetadata
and the UpdateTokenMetadata struct initialization accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d347aa2c-54ca-4797-9b86-f68d03a6a545
⛔ Files ignored due to path filters (2)
x/feeabstraction/types/params.pb.gois excluded by!**/*.pb.gox/feeabstraction/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
CHANGELOG.mdproto/kiichain/feeabstraction/v1beta1/params.protoproto/kiichain/feeabstraction/v1beta1/tx.protox/feeabstraction/keeper/msg_server.gox/feeabstraction/keeper/msg_server_test.gox/feeabstraction/types/msg.gox/feeabstraction/types/msg_test.gox/feeabstraction/types/params.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
x/feeabstraction/README.md (1)
184-184: Clarify TWAP timing to “next BeginBlocker cycle.”Line 184 could be read as immediate recomputation. Suggest wording like “prices are set to zero so the next BeginBlocker cycle adopts current TWAP values.”
Based on learnings:
InitGenesisintentionally resets token prices to zero because TWAP is recomputed inBeginBlocker, and stale carried prices are intentionally avoided.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/feeabstraction/README.md` at line 184, Update the sentence to clarify timing: replace "Prices are always set to zero so that BeginBlocker adopts the current TWAP." with wording such as "Prices are set to zero so the next BeginBlocker cycle adopts current TWAP values." Also mention that InitGenesis intentionally resets token prices to zero because TWAP is recomputed in BeginBlocker and this avoids carrying stale prices, referencing InitGenesis, BeginBlocker, and TWAP to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@x/feeabstraction/README.md`:
- Line 184: Update the sentence to clarify timing: replace "Prices are always
set to zero so that BeginBlocker adopts the current TWAP." with wording such as
"Prices are set to zero so the next BeginBlocker cycle adopts current TWAP
values." Also mention that InitGenesis intentionally resets token prices to zero
because TWAP is recomputed in BeginBlocker and this avoids carrying stale
prices, referencing InitGenesis, BeginBlocker, and TWAP to make the intent
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef09a0b1-718c-4fd2-81fe-9fea9f38c462
📒 Files selected for processing (1)
x/feeabstraction/README.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
x/feeabstraction/keeper/msg_server.go (1)
116-123:⚠️ Potential issue | 🟠 MajorPreserve
token.Enabledwhen rebuilding the zero-price collection.This rewrite intentionally zeroes
Price, but it also drops theEnabledvalue fromUpdateTokenMetadata. As written, governance cannot reliably persist a token's enabled/disabled state throughUpdateFeeTokens.🐛 Proposed fix
resetItems := make([]types.FeeTokenMetadata, len(msg.Tokens.Items)) for i, token := range msg.Tokens.Items { - resetItems[i] = types.NewFeeTokenMetadata(token.Denom, token.OracleDenom, token.Decimals, math.LegacyZeroDec()) + resetItems[i] = types.FeeTokenMetadata{ + Denom: token.Denom, + OracleDenom: token.OracleDenom, + Decimals: token.Decimals, + Price: math.LegacyZeroDec(), + Enabled: token.Enabled, + } } resetFeeTokens := types.NewFeeTokenMetadataCollection(resetItems...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x/feeabstraction/keeper/msg_server.go` around lines 116 - 123, When rebuilding resetItems from msg.Tokens.Items you currently drop the Enabled flag; preserve each token's Enabled state by carrying token.Enabled into the new metadata before calling ms.FeeTokens.Set. Specifically, in the loop that builds resetItems (iterating msg.Tokens.Items) create the zero-priced metadata via types.NewFeeTokenMetadata(...) and then set the Enabled field from the original token (e.g. m := types.NewFeeTokenMetadata(...); m.Enabled = token.Enabled; resetItems[i] = m), or use an available constructor that accepts Enabled, so types.FeeTokenMetadata entries keep their Enabled value when you create resetFeeTokens and call ms.FeeTokens.Set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@x/feeabstraction/keeper/msg_server.go`:
- Around line 116-123: When rebuilding resetItems from msg.Tokens.Items you
currently drop the Enabled flag; preserve each token's Enabled state by carrying
token.Enabled into the new metadata before calling ms.FeeTokens.Set.
Specifically, in the loop that builds resetItems (iterating msg.Tokens.Items)
create the zero-priced metadata via types.NewFeeTokenMetadata(...) and then set
the Enabled field from the original token (e.g. m :=
types.NewFeeTokenMetadata(...); m.Enabled = token.Enabled; resetItems[i] = m),
or use an available constructor that accepts Enabled, so types.FeeTokenMetadata
entries keep their Enabled value when you create resetFeeTokens and call
ms.FeeTokens.Set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9d91311-847e-4e2f-a3ae-65284ee3d240
📒 Files selected for processing (1)
x/feeabstraction/keeper/msg_server.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
17-23: Clarify lifecycle wording and use canonical type names in changelog entries.These bullets are directionally correct, but the phrasing can be more precise: prices are reset to zero during metadata update and then populated by the oracle/TWAP cycle; also prefer
UpdateFeeTokens/UpdateTokenMetadatanaming for consistency.✍️ Suggested wording
-- Ensure feeTokenMetadata initial prices after updateFeeTokenMetadata is picked up from oracle +- Ensure fee token metadata prices are reset on `UpdateFeeTokens` and populated by the next oracle/TWAP cycle ### Removed -- Removed price field input in updateTokenMetadata request +- Remove `price` field input from `UpdateTokenMetadata` requestBased on learnings:
InitGenesisand fee abstraction intentionally start prices at zero so oracle TWAP recomputes authoritative prices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 17 - 23, Update the changelog bullets to use the canonical names UpdateFeeTokens and UpdateTokenMetadata, clarify that fee token prices are reset to zero during metadata updates (and then populated by the oracle/TWAP cycle), and explicitly mention that InitGenesis and the fee abstraction intentionally start prices at zero so TWAP recomputes authoritative prices; keep the other entries referencing DecCoins.Validate in RewardPool.ValidateGenesis and denom consistency in GenesisState.Validate with Params.TokenDenom but rephrase to use the canonical type/function names and precise lifecycle wording accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 17-23: Update the changelog bullets to use the canonical names
UpdateFeeTokens and UpdateTokenMetadata, clarify that fee token prices are reset
to zero during metadata updates (and then populated by the oracle/TWAP cycle),
and explicitly mention that InitGenesis and the fee abstraction intentionally
start prices at zero so TWAP recomputes authoritative prices; keep the other
entries referencing DecCoins.Validate in RewardPool.ValidateGenesis and denom
consistency in GenesisState.Validate with Params.TokenDenom but rephrase to use
the canonical type/function names and precise lifecycle wording accordingly.
2 participants
Description
Type of change
How Has This Been Tested?
Updated tests
PR Checklist:
Make sure each step was done:
make lint-fix